Only rename method type params shadowing enclosing class type params #24684

tanishiking · 2025-12-06T17:43:29Z

Previously, method type parameters were always renamed if they had the same name as any class type parameter, regardless of whether the class type parameter was actually used in the method signature.

As a result, we rename methods's type params for unnecessary names like: #24671 (Note that renaming actually doesn't cause binary incompatibility, and this change is just for make generic signature a bit clear and silence false-positive MIMA reporting).

For example, in an example below, the Scala compiler rename the generic signature of bar to something like bar[T1](x: T1): T1 because T is used by Foo[T]. However, this is unnessary rename because none of T in method signature refer the T of Foo[T].

class Foo[T]:
  def bar[T](x: T): T = ???

This commit makes the renaming conditional:
Method type parameters are only renamed when

(1) A class type parameter is referenced in the method signature, and
(2) That class type parameter is shadowed by a method type parameter

Previously, method type parameters were always renamed if they had the same name as any class type parameter, regardless of whether the class type parameter was actually used in the method signature. As a result, we rename methods's type params for unnecessary names like: scala#24671 (Note that renaming actually doesn't cause binary incompatibility, and this change is just for make generic signature a bit clear and silence false-positive MIMA reporting). For example, in an example below, the Scala compiler rename the generic signature of `bar` to something like `bar[T1](x: T1): T1` because `T` is used by `Foo[T]`. However, this is unnessary rename because none of T in method signature refer the `T` of `Foo[T]`. ```scala class Foo[T]: def bar[T](x: T): T = ??? ``` This commit makes the renaming conditional: Method type parameters are only renamed when - (1) A class type parameter is referenced in the method signature, and - (2) That class type parameter is shadowed by a method type parameter

tanishiking · 2025-12-07T12:33:37Z

tests/generic-java-signatures/shadowedTypeParam.scala

+  // copy$default$1 still have `<T1> T Bar.copy$default$1` rather than `<T> T Bar.copy$default$1`
+  // as reported in https://github.com/scala/scala3/issues/24671
+  // The type parameter rename occurs because the return type T refers the enclosing class's type param T.
+  // printMethodSig(barMethods, "copy$default$1")


Still MiMa won't be happy with <T1> T Bar.copy$default$1 rather than <T> T Bar.copy$default$1 (but the original signature was wrong, the return type T is shadowed by method type parameter.)

Ah! I didn't pay attention. A small reproducer is

abstract class C[T] { def x: T def m[T] = x }

We have that wrong in Scala 2, I created scala/bug#13144.

Thanks! that's a good reproducer :)
It seems like renaming to <T1:Ljava/lang/Object;>()TT; is right thing to do 👍

lrytz

false-positive MIMA reporting

nitpicking.. it's not really a false positive report by mima, but mima can be configured to check changes in the classfile (generic signatures) that go beyond binary compatibility.

lrytz · 2025-12-08T09:05:24Z

tests/generic-java-signatures/shadowedTypeParam.scala

+  // copy$default$1 still have `<T1> T Bar.copy$default$1` rather than `<T> T Bar.copy$default$1`
+  // as reported in https://github.com/scala/scala3/issues/24671
+  // The type parameter rename occurs because the return type T refers the enclosing class's type param T.
+  // printMethodSig(barMethods, "copy$default$1")


Ah! I didn't pay attention. A small reproducer is

abstract class C[T] { def x: T def m[T] = x }

We have that wrong in Scala 2, I created scala/bug#13144.

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

lrytz · 2025-12-08T09:58:42Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

+    val usedMethodTypeParamNames = collection.mutable.Set.empty[Name]
+    val usedClassTypeParams = collection.mutable.Set.empty[Symbol]
+
+    def collect(tp: Type): Unit = tp.dealias match


In Scala 2 we have a tp.foreach, isn't there something similar? https://github.com/scala/scala/blob/41f6cfcd4b05298c59fc375c27b21eaea8e09653/src/reflect/scala/reflect/internal/Types.scala#L782

Ah, we have

scala3/compiler/src/dotty/tools/dotc/core/Types.scala

Lines 7067 to 7071 in ecb1fd3

abstract class TypeTraverser(using Context) extends TypeAccumulator[Unit] {

def traverse(tp: Type): Unit

def apply(x: Unit, tp: Type): Unit = traverse(tp)

protected def traverseChildren(tp: Type): Unit = foldOver((), tp)

}

👍

scala/bug#13144

lrytz

LGTM otherwise

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

lrytz · 2025-12-15T15:23:51Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

+          else if sym.isTypeParam && sym.isContainedIn(initialSymbol.topLevelClass) then
+            usedClassTypeParams += sym
+          else
+            traverse(pre)


this should probably not be in the else branch?

Right, we don't need to traverse the pre

Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>

tanishiking · 2025-12-17T09:17:43Z

Thanks @lrytz ! Could you take an another look? (Added a change based on your suggestion)

lrytz

I don't want to be annoying, so feel free to merge this, LGTM.

lrytz · 2025-12-17T09:14:12Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

      )

+  private def isTypeParameterInMethSig(sym: Symbol, initialSymbol: Symbol)(using Context) =
+    !sym.maybeOwner.isTypeParam &&


This condition I don't understand, is it necessary?

lrytz · 2025-12-17T09:15:09Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

+  private def isTypeParameterInMethSig(sym: Symbol, initialSymbol: Symbol)(using Context) =
+    !sym.maybeOwner.isTypeParam &&
+      sym.isTypeParam && (
+        (initialSymbol.is(Method) && initialSymbol.typeParams.contains(sym))


initialSymbol is always a method.. could this be sym.owner == initialSymbol?

lrytz · 2025-12-17T09:15:53Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

        (initialSymbol.is(Method) && initialSymbol.typeParams.contains(sym))
      )

+  private def isTypeParameterInMethSig(sym: Symbol, initialSymbol: Symbol)(using Context) =


I would probably nest this method within collectUsedTypeParams

lrytz · 2025-12-17T09:18:20Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

+        val sym = tp.typeSymbol
+        if isTypeParameterInMethSig(sym, initialSymbol) then
+          usedMethodTypeParamNames += sym.name
+        else if sym.isTypeParam && sym.isContainedIn(initialSymbol.topLevelClass) then


would sym.isTypeParam && sym.owner == initialSymbol.topLevelClass also be correct?

lrytz · 2025-12-17T10:43:12Z

compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala

+        if isTypeParameterInMethSig(sym, initialSymbol) then
+          usedMethodTypeParamNames += sym.name


do we even need this case, aren't all references to the method's type params handled above?

tanishiking force-pushed the dont-rename branch from d190111 to a93c849 Compare December 7, 2025 11:42

tanishiking commented Dec 7, 2025

View reviewed changes

tanishiking marked this pull request as ready for review December 7, 2025 12:35

tanishiking mentioned this pull request Dec 8, 2025

MIMA Binary compatibility breakage in case class copy method default getter upgrading from Scala 3.7.4 -> 3.8.0-RC3 #24671

Open

lrytz reviewed Dec 8, 2025

View reviewed changes

lrytz mentioned this pull request Dec 8, 2025

Incorrect generic signature with shadowing type parameter scala/bug#13144

Open

Gedochao added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Dec 8, 2025

Gedochao added this to the 3.8.0 milestone Dec 8, 2025

tanishiking added 2 commits December 8, 2025 22:03

refactor: use TypeTraverser to collect used type params

3d4d397

Add another test case that shadows class's type param

9b85e44

scala/bug#13144

tanishiking mentioned this pull request Dec 8, 2025

Fix generic signatures to collect type params from all enclosing classes #24621

Draft

tanishiking requested a review from lrytz December 9, 2025 02:34

Gedochao assigned lrytz Dec 10, 2025

lrytz reviewed Dec 15, 2025

View reviewed changes

Use Type.foreachPart collectUsedTypeParams

7cc63d4

Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>

tanishiking force-pushed the dont-rename branch from 27f5e3e to 7cc63d4 Compare December 17, 2025 07:39

tanishiking requested a review from lrytz December 17, 2025 09:17

lrytz approved these changes Dec 17, 2025

View reviewed changes

	abstract class TypeTraverser(using Context) extends TypeAccumulator[Unit] {
	def traverse(tp: Type): Unit
	def apply(x: Unit, tp: Type): Unit = traverse(tp)
	protected def traverseChildren(tp: Type): Unit = foldOver((), tp)
	}

		if isTypeParameterInMethSig(sym, initialSymbol) then
		usedMethodTypeParamNames += sym.name

Only rename method type params shadowing enclosing class type params #24684

Are you sure you want to change the base?

Only rename method type params shadowing enclosing class type params #24684

Conversation

tanishiking commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanishiking Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanishiking Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanishiking commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tanishiking commented Dec 6, 2025 •

edited

Loading

tanishiking Dec 7, 2025 •

edited

Loading

tanishiking Dec 8, 2025 •

edited

Loading

tanishiking commented Dec 17, 2025 •

edited

Loading